zipper_algebra: further specialize grafting - #62
Conversation
This leads to a few percent speed-up in the benchmarks
|
Let's fold do-graft optimization into the masked version @marcin-rzeznicki |
|
@Adam-Vandervorst this is done in 353f4d4 @luketpeterson do you think this is the right place for the optimization? Also, I tried to optimize it further by doing the following: - match child_mask.count_bits() {
+ let effective_mask = child_mask & src.child_mask();
+ match effective_mask.count_bits() {
0 => {
if remove_unset {
self.remove_branches(false);
+ } else {
+ self.remove_unmasked_branches(child_mask.not(), false);
}
}
1 => {
if remove_unset {
self.remove_branches(false);
+ } else {
+ self.remove_unmasked_branches(child_mask.not(), false);
}
- let byte = child_mask.indexed_bit::<true>(0).expect("one bit set");
+ let byte = effective_mask.indexed_bit::<true>(0).expect("one bit set");
self.descend_to_byte(byte);
self.graft_src_at(src, &[byte]);
self.ascend_byte();
}
2 => {
if remove_unset {
self.remove_branches(false);
+ } else {
+ self.remove_unmasked_branches(child_mask.not(), false);
}
- let first_byte = child_mask.indexed_bit::<true>(0).expect("some bit set");
+ let first_byte = effective_mask.indexed_bit::<true>(0).expect("some bit set");
self.descend_to_byte(first_byte);
self.graft_src_at(src, &[first_byte]);
self.ascend_byte();
- let second_byte = child_mask.next_bit(first_byte).expect("two bits set");
+ let second_byte = effective_mask.next_bit(first_byte).expect("two bits set");But this fails miserably in some of the tests by triggering the assertion in @luketpeterson why would the assertion fail? |
Fixing issue with remove_unmasked_branches at non-existent paths, and adding test
Your code exposed a bug affecting non-existent paths. I pushed a fix.
Unfortunately, that approach is slower. To focus on the case where it would matter, I created a synthetic benchmark where I was passing a variable child_mask, but grafting from a source with 0, 1, or 2 downstream branches. So the effective mask would go down one of those cases. Here are the results on my laptop. So it does pull ahead when graft_masked_branches is being used as a glorified remove. But using it this way is an anti-pattern. It works, but it's not what the API was designed for. So sacrificing performance in the intended use case to improve an improper use case is not a win. If the user knows they are masking out far more branches than they intend to graft, they can do the |
luketpeterson
left a comment
luketpeterson
left a comment
There was a problem hiding this comment.
I already wrote some comments / made the tweaks I had in mind.
|
Thank you, @luketpeterson! I agree with your reasoning 100%. I'll push the relevant code (that keeps the masks reasonable by intersecting them) to the zipper_algebra module based on this. |
3 participants
This leads to a few percent speed-up in the benchmarks. For instance, for symmetric difference simulation, before this we had:
and now we have:
so there is a quite considerable speed-up (especially for longer paths)